Before and After hooks. #31
Conversation
|
Hi Tristan, I only had a quick look at what you did but it looks pretty good so far. As a general suggestion, I think you should stick with the domain term hook instead of callback. A callback is any function that should be called by another function. In Cucumber's vocabulary, a hook is a specific callback being fired when certain events occur during feature execution. Does it make sense? I'll give you more fine-grained feedback tonight. Thanks for the nice work! |
| invoke: function(world, callback) { | ||
| try { | ||
| code.apply(world, [callback]); | ||
| } catch(exception) { |
There was a problem hiding this comment.
I'm not sure how exceptions should be handled here, if they should at all. I discussed it a few minutes with @msassak and we thought exceptions within hooks should not be handled. But after some trials on Cucumber 1.1, it seems they actually are: they halt the current scenario but not the whole Cucumber thread.
What's certain is that there is no such behaviour in the hooks.feature file. I.e. this part of the code is not tested :) I suggest to remove this catch block for now, until we know exactly what to do.
There was a problem hiding this comment.
Removing it seems reasonable since it isn't currently defined.
|
I think there is a conceptual mistake in the current implementation. Before and After hooks are supposed to be fired before and after scenarios. If I get it right, the hooks are currently fired before and after features. Features are actually nonexistent when it comes to hooks. You can only hook around steps, scenarios and the whole suite run. Let's get back to this project of yours for a minute. Do you actually need suite, feature or scenario hooks there? |
|
Wow, not sure how I missed that. I'll get it changed to scenario soon. Ideally I need before and after scenario with tags, but I can get away without tags or even at the suite level for now. |
| afterHook.invoke(world, callback) | ||
| }, function() { | ||
| if (callback) { | ||
| callback(); |
There was a problem hiding this comment.
Nested functions quickly become hard to read, IMO. I've used the extract method refactoring pattern in several spots (e.g. just like in the AstTreeWalker). Basically you move down the first nested callback to a wrapSomething() function and repeat that extraction until the end of the callback chain.
It's not perfect but I think it reduces the noise and complexity of such code fragments.
WDYT?
There was a problem hiding this comment.
Definitely agree. I knew this part was going to need some clean up. I was already thinking about switching to
supportCodeLibrary.triggerBeforeHooks(world, function() {
scenario.acceptVisitor(self, function() {
supportCodeLibrary.triggerAfterHooks(world, callback);
});
});Combining that with extracting the callbacks it'll be a lot cleaner and substantially easier to test.
|
Nice work! Thanks Tristan. The next step would be to go green against the related hooks.feature's scenarios. You've added JS step definitions to features/step_definitions/cucumber_steps.js, which is a good start. However, hooks.feature is part of the cucumber-features suite. Implementations are expected to pass against the cucumber-features suite run by Cucumber-ruby. Running it through the implementation itself is a secondary objective (but definitely great and important). In short, the step definitions are to be written in Ruby, in The step definitions there generally call a single method that is called a mapping. A few of them are common to all implementations (see cucumber_mappings.rb) while the others are specific to implementations (in cucumber.js, you'll find them in features/step_definitions/cucumber_js_mappings.rb). As you'd have to send pull requests on cucumber-features, I suggest you create a temporary I hope this is not too confusing. Feel free to ping me if you need help! |
|
I assume I need to extract some of the step definitions to the hook mappings, as suggested, but am I on the right track? |
|
You're on the right track indeed. Excellent work so far! I've refactored the step definitions a bit to be more consistent with the rest of them. I suggest you merge these changes into the pull request. Have a look at it and feel free to ask questions if you have any. |
|
This has fallen pretty far behind master. Am I missing anything before I attempt to rebase my branch? |
|
There were no major changes since your last commit. I think you should be able to merge/rebase quite easily. |
This is so they will run in LIFO order, as defined by the Cucumber specification.
Also added unit test coverage for invoking before and after hooks.
|
That was easier than expected. Let me know if you see anything funky. |
|
Any further progress on this? |
|
@dbrock I'm now reviewing the latest commits, it's a highly requested feature. |
|
I've been rather quiet lately, but let me know if there's anything else I can help out with. |
| supportCodeLibrary.triggerBeforeHooks(world, function() { | ||
| scenario.acceptVisitor(self, function() { | ||
| supportCodeLibrary.triggerAfterHooks(world, function() { | ||
| if (callback) { |
There was a problem hiding this comment.
I want to say I originally added this to get a test passing, but not 100% sure. If that's the case then the test case should be improved and this should be removed.
There was a problem hiding this comment.
I'm in the middle of a merge from master onto your branch and I removed this condition. It seems to work well.
|
+1. very much looking forward to this addition to wire in database cleaner after tests. |
|
@tristandunn Thank you for your nice work on this. |
|
Awesome! Glad to help. |
|
great work guys - fantastic to see a much requested feature arrive so quickly. |
|
Well it actually took a long time to get merged, mostly because of a crazy end of year on my side ;) We still need to implement the other types of hooks though (see #32). |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Before and After hooks would help clean up some code in my pig project, so I'm attempting to add them. I have most of it working and tested, but it got a little tricky towards the end. I'd appreciate some feedback on whether I am heading in the right direction or any suggestions on how to improve the implementation.